Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve ruff config #15516

Merged
merged 5 commits into from
Jun 28, 2023
Merged

Improve ruff config #15516

merged 5 commits into from
Jun 28, 2023

Conversation

AlexWaygood
Copy link
Member

  • Add fix = true to pyproject.toml. This means ruff will apply autofixes when possible if ruff . is run (and will apply autofixes to PRs).
  • Remove isort as a test dependency; just use ruff instead. It has the same import-sorting style, is faster, and requires less configuration.
  • Remove an outdated paragraph in CONTRIBUTING.md about how you'll get a CI failure if you don't run black and isort before filing a PR. This is no longer true since we switched to using pre-commit.ci.

@github-actions

This comment has been minimized.

Sign in to view
hauntsaninja
hauntsaninja reviewed Jun 25, 2023
View reviewed changes
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to have fix on by default, can we mark the following as unfixable:

unfixable = [
    "F841",  # unused variable. ruff keeps the call, but mostly we want to get rid of it all
    "F601",  # automatic fix might obscure issue
    "F602",  # automatic fix might obscure issue
    "B018",  # automatic fix might obscure issue
]

@github-actions

This comment has been minimized.

Sign in to view
@ikonst
Copy link
Contributor

ikonst commented Jun 26, 2023 •
edited
Loading

So I gather they "autofix" B018 ("Found useless expression. Either assign it to a variable or remove it.") by ... removing the expression rather than assigning it?

Update: https://beta.ruff.rs/docs/rules/#flake8-bugbear-b doesn't suggest B018 has a fixer currently. Are you adding it just as a precaution?

@AlexWaygood
Copy link
Member Author

Update: https://beta.ruff.rs/docs/rules/#flake8-bugbear-b doesn't suggest B018 has a fixer currently. Are you adding it just as a precaution?

I just went with Shantanu's suggestions here, but I definitely agree that it's good in general to be cautious with autofixes, and I agree that if they were to add an autofix for B018, we probably wouldn't want it

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jun 26, 2023 •
edited
Loading

Oh hmmm I don't remember why I marked B018 unfixable, possibly just defensive, possibly I just looked at some undesirable change and got confused about what "fixed" it. I added ruff to my work codebase a couple weeks ago and my suggestion^ is just whatever I'd decided was good at that time.

In case you're curious, this is what I have currently at work:

select = [
    "E", "F", "W", "B", "C4", "ASYNC", "PIE",
    # grab bag of random things
    "DTZ003", "DTZ004",
    "PLE1310", "PLW0120",
    "SIM101", "SIM110", "SIM201", "SIM202", "SIM222", "SIM223",
    "S506",
    "RET501", "RET502",  # RET503 is also nice, but there's currently some noise
    "RUF006", "RUF200",
    "COM819",
]
ignore = [
    "E402",   # people usually know what they're doing
    "E501",   # line too long. black does a good enough job
    "E731",   # lambda expression assignments. these are nice sometimes
    "E741",   # variable names like "l"
    "B011",   # assert false. we don't use python -O
    "C408",   # we often use dict with keyword args
]
unfixable = [
    "F841",  # unused variable. ruff keeps the call, but mostly we want to get rid of it all
    "F601",  # automatic fix might obscure issue
    "F602",  # automatic fix might obscure issue
    "B018",  # automatic fix might obscure issue
    "SIM222",  # automatic fix might obscure issue
    "SIM223",  # automatic fix might obscure issue
]

@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@hauntsaninja hauntsaninja merged commit 4012c50 into python:master Jun 28, 2023
@AlexWaygood AlexWaygood deleted the more-ruff branch June 28, 2023 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hauntsaninja hauntsaninja hauntsaninja approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AlexWaygood @ikonst @hauntsaninja